[UIKit] Update for Xcode10. #4253

mandel-macaque · 2018-06-14T07:19:38Z

This is an update for the Xcode 10. I am interested in input regarding the addition of the new protocols that have been added with old methods (already present) and new ones:

UIUserActivityRestoring: "void RestoreUserActivityState (NSUserActivity userActivity)" was already present.
UIFocusItemScrollableContainer: "ContentOffset" and "ContentSize" already present.
UIFocusItemContainer: "Frame" already present.

I added the methods in the protocols with the correct signature, left the methods in the old interfaces and added new to ignore errors. Comments are welcome regarding this approach.

Diffs are:

rolfbjarne · 2018-06-14T08:00:46Z

src/UIKit/UIEnums.cs

+	[Native]
+	public enum UIPrintErrorCode : long
+	{
+		ingNotAvailableError = 1,


Ups, sorry.

rolfbjarne · 2018-06-14T08:01:20Z

src/spritekit.cs

@@ -149,8 +149,13 @@ partial interface SKNode : NSSecureCoding, NSCopying {
 		[return: NullAllowed]
 		SKNode Create (string filename);

+		// new is needed after iOS 12 due to the new UIFocustItem protocol.


I suppose the comment should say UIFocusItem?

rolfbjarne · 2018-06-14T08:03:21Z

src/uikit.cs

@@ -16687,6 +16732,15 @@ interface UIFocusItem : UIFocusEnvironment
 		[Abstract]
 		[Export ("canBecomeFocused")]
 		bool CanBecomeFocused { get; }
+
+		[TV (12, 0), iOS (12, 0), NoWatch]
+		[Abstract]


This is a breaking change: can't add new [Abstract] members to an existing protocol.

It should go into a #if XAMCORE_4_0 block.

rolfbjarne · 2018-06-14T08:05:00Z

src/uikit.cs

+
+
+		[TV (12, 0), iOS (12, 0)]
+		[Abstract]


Breaking change.

So XAMCORE_4_0 then

rolfbjarne · 2018-06-14T08:05:08Z

src/uikit.cs

+		IUIFocusEnvironment ParentFocusEnvironment { get; }
+
+		[TV (12, 0), iOS (12, 0)]
+		[Abstract]


Breaking change.

rolfbjarne · 2018-06-14T08:06:36Z

src/uikit.cs

+	interface UIFocusItemScrollableContainer : UIFocusItemContainer
+	{
+		[Abstract]
+		[TV(9,0), NoWatch]


The protocol itself is TV (12,0), but there are members with TV (9,0)? That sounds strange (but otoh it wouldn't surprise me either...)

yeah, the protocol was added later, let me double check.

monojenkins · 2018-06-14T09:04:55Z

Build failure
Build comment file:

✅ Provisioning succeeded
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only)
✅ Generator Diff
🔥 Test run failed 🔥

Test results

4 tests failed, 60 tests passed.

Failed tests

introspection/iOS Unified 32-bits - simulator/Debug: Failed
introspection/iOS Unified 64-bits - simulator/Debug: Failed
introspection/tvOS - simulator/Debug: Failed
introspection/watchOS - simulator/Debug: Failed

mandel-macaque · 2018-06-14T11:45:55Z

@rolfbjarne changes pushed, added the #is statements and filled the rdars that have been added in the comments and in the trello board.

monojenkins · 2018-06-14T11:50:13Z

Build failure
Build comment file:

✅ Provisioning succeeded

rolfbjarne · 2018-06-14T12:18:34Z

src/uikit.cs

+
+		// FIXME: declared as a @required, but this breaks compatibilit
+		// Radar: 41121416
+#if XAMCORE_4_0 


Only the [Abstract] need to be in XAMCORE_4_0:

#if XAMCORE_4_0 [Abstract] #endif [TV (12, 0), iOS (12, 0), NoWatch] [Export ("frame")] CGRect Frame { get; }

rolfbjarne · 2018-06-14T12:19:23Z

src/uikit.cs

@@ -5235,7 +5235,7 @@ interface UIDocument : NSFilePresenter, NSProgressReporting {

 		[iOS (8,0)]
 		[Export ("restoreUserActivityState:")]
-		void RestoreUserActivityState (NSUserActivity userActivity);
+		new void RestoreUserActivityState (NSUserActivity userActivity);


Can you try locally to remove this method, so that we get the one from the protocol, and see what happens to the api diff? I think it won't be a breaking change, in which case it's better to remove it.

monojenkins · 2018-06-14T12:36:58Z

Build failure
Build comment file:

✅ Provisioning succeeded
🔥 Build failed 🔥

spouliot

Both intro and API Diff are reporting several issues.

spouliot · 2018-06-14T13:06:13Z

src/uikit.cs

@@ -6133,6 +6133,10 @@ interface UIGraphicsImageRendererFormat
 		[Export ("opaque")]
 		bool Opaque { get; set; }

+		[Introduced (PlatformName.iOS, 10, 0, message: "Use the 'PreferredRange' property instead.")]


you don't need the Introduced here

spouliot · 2018-06-14T13:06:18Z

src/uikit.cs

@@ -6133,6 +6133,10 @@ interface UIGraphicsImageRendererFormat
 		[Export ("opaque")]
 		bool Opaque { get; set; }

+		[Introduced (PlatformName.iOS, 10, 0, message: "Use the 'PreferredRange' property instead.")]
+		[Deprecated (PlatformName.iOS, 12, 0, message: "Use the 'PreferredRange' property instead.")]
+		[Introduced (PlatformName.TvOS, 10, 0, message: "Use the 'PreferredRange' property instead.")]


spouliot · 2018-06-14T13:08:26Z

src/uikit.cs

+		CGSize VisibleSize { get; }
+	}
+
+	[iOS (8,0), TV (8,0), NoWatch] // it was added on 8,0, but was not binded and the method was added in 12,0


tvOS started with 9.0 so remove it's attribute (which means it's always been available)

spouliot · 2018-06-14T13:09:08Z

src/uikit.cs

+		[Field ("UIKitVersionString")]
+		NSString UIKitVersionString { get; }
+
+	}


remove those and put them in UIKit.ignore
we're doing the same in other semi-versioned frameworks

dalexsoto

Looks good to me after Sebastien and Rolf comments are addressed.

VincentDondain · 2018-06-14T19:20:56Z

src/spritekit.cs

@@ -149,8 +149,10 @@ partial interface SKNode : NSSecureCoding, NSCopying {
 		[return: NullAllowed]
 		SKNode Create (string filename);

+#if MONOMAC || WATCH


I don't understand that change. I believe this is a breaking change. Also I'm confused about a SpriteKit change in this UIKit PR.

No, it is not. In iOS and TV we have the IUIFocusItem interface that already contains the Frame property. That means that in those platforms, it can be removed.

On WatchOS and MacOS we do not have that interface, therefore the property does not get generated and you have to add it manually.

@VincentDondain once the build is done, take a look at the api diff and the Extrospection results both confirm my statement :)

Ah got it, didn't realize it was in IUIFocusItem 👍

monojenkins · 2018-06-14T21:11:10Z

Build failure
Build comment file:

✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (🔥 breaking changes 🔥)
✅ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

2 tests failed, 62 tests passed.

Failed tests

introspection/iOS Unified 32-bits - simulator/Debug: Failed
introspection/iOS Unified 64-bits - simulator/Debug: Failed

monojenkins · 2018-06-15T06:40:25Z

Build failure
Build comment file:

✅ Provisioning succeeded

monojenkins · 2018-06-15T10:49:44Z

Build failure
Build comment file:

✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (🔥 breaking changes 🔥)
✅ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

1 tests failed, 63 tests passed.

Failed tests

introspection/iOS Unified 32-bits - simulator/Debug: Failed

spouliot

This looks good but please check/fix/comment on bot failures before merging.

VincentDondain

LGTM when breaking change is addressed (:

VincentDondain · 2018-06-15T15:20:45Z

src/uikit.cs

@@ -16687,6 +16719,19 @@ interface UIFocusItem : UIFocusEnvironment
 		[Abstract]
 		[Export ("canBecomeFocused")]
 		bool CanBecomeFocused { get; }
+
+		// FIXME: declared as a @required, but this breaks compatibilit


Minor typo: "compatibility"

VincentDondain · 2018-06-15T15:23:35Z

src/uikit.cs

@@ -9862,10 +9872,6 @@ interface UIResponder : UIAccessibilityAction, UIAccessibilityFocus
 		[Export ("updateUserActivityState:")]
 		void UpdateUserActivityState (NSUserActivity activity);

-		[iOS (8,0)]
-		[Export ("restoreUserActivityState:")]
-		void RestoreUserActivityState (NSUserActivity activity);


Ah the argument here was activity and not userActivity therefore removing it and using the interface is a breaking change...

mmm, we have allowed parameter name changes in the past, but since the UIUserActivityRestoring protocol just got added lets just rename userActivity to activity to avoid the breaking change.

Sure, also, braking change is probably a too "strong" adjective. API will change, not break ;)

Doing it nevertheless

It is a breaking change because of named parameters for example if anyone is doing RestoreUserActivityState (activity: x); it would not compile anymore, but yeah not very likely

rolfbjarne · 2018-06-15T15:30:41Z

src/uikit.cs

 		[Export ("contentSize")]
-		CGSize ContentSize { get; set; }
+		new CGSize ContentSize { get; set; }


Can this be removed as well, so that we get it from UIFocusItemScrollableContainer instead, or will the api diff complain?

No, in this case the protocol just exposes the get, but not the set.

rolfbjarne · 2018-06-15T15:30:49Z

src/uikit.cs

@@ -12995,7 +12998,7 @@ interface UIView : UIAppearance, UIAppearanceContainer, UIAccessibility, UIDynam
 		CoreAnimation.CALayer Layer { get; }

 		[Export ("frame")]
-		CGRect Frame { get; set; }
+		new CGRect Frame { get; set; }


Same: can this be removed as well, so that we get it from UIFocusItemContainer instead, or will the api diff complain?

Same, protocol just exposes get, not set.

dalexsoto · 2018-06-15T15:54:53Z

src/uikit.cs

@@ -9862,10 +9872,6 @@ interface UIResponder : UIAccessibilityAction, UIAccessibilityFocus
 		[Export ("updateUserActivityState:")]
 		void UpdateUserActivityState (NSUserActivity activity);

-		[iOS (8,0)]
-		[Export ("restoreUserActivityState:")]
-		void RestoreUserActivityState (NSUserActivity activity);


mmm, we have allowed parameter name changes in the past, but since the UIUserActivityRestoring protocol just got added lets just rename userActivity to activity to avoid the breaking change.

dalexsoto · 2018-06-15T15:55:35Z

src/uikit.cs

+		[Abstract]
+		[iOS (8,0), TV(12,0)]
+		[Export ("restoreUserActivityState:")]
+		void RestoreUserActivityState (NSUserActivity userActivity);


Rename userActivity to activity to avoid a breaking change.

…ios into uikit-xcode10

monojenkins · 2018-06-18T08:47:48Z

Build success
Build comment file:

✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (🔥 breaking changes 🔥)
✅ Generator Diff (please review changes)
✅ Test run succeeded

monojenkins · 2018-06-18T12:14:29Z

Build success
Build comment file:

✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (🔥 breaking changes 🔥)
✅ Generator Diff (please review changes)
✅ Test run succeeded

[UIKit] Update for Xcode10.

07355ad

mandel-macaque requested review from dalexsoto, rolfbjarne, spouliot and VincentDondain as code owners June 14, 2018 07:19

rolfbjarne requested changes Jun 14, 2018

View reviewed changes

Address reviews.

df9d8b7

Removed not needed comment.

721efdc

rolfbjarne requested changes Jun 14, 2018

View reviewed changes

spouliot suggested changes Jun 14, 2018

View reviewed changes

Address comments from reviews.

a651156

dalexsoto approved these changes Jun 14, 2018

View reviewed changes

VincentDondain reviewed Jun 14, 2018

View reviewed changes

mandel-macaque added 2 commits June 15, 2018 07:58

Add exception for the tests after double checking the headers.

9464027

Merge branch 'xcode10' into uikit-xcode10

6a08c36

spouliot approved these changes Jun 15, 2018

View reviewed changes

VincentDondain approved these changes Jun 15, 2018

View reviewed changes

rolfbjarne reviewed Jun 15, 2018

View reviewed changes

dalexsoto requested changes Jun 15, 2018

View reviewed changes

mandel-macaque added 2 commits June 18, 2018 08:16

Ignore a protocol test on 32 devices.

462bdc3

Merge branch 'uikit-xcode10' of github.com:mandel-macaque/xamarin-mac…

e9fad57

…ios into uikit-xcode10

rolfbjarne approved these changes Jun 18, 2018

View reviewed changes

Update param name, fix typo.

b97f667

dalexsoto approved these changes Jun 18, 2018

View reviewed changes

mandel-macaque merged commit 553238a into dotnet:xcode10 Jun 18, 2018

VincentDondain mentioned this pull request Oct 1, 2018

ContentOffset only available in iOS 12 or newer #4893

Closed

[UIKit] Update for Xcode10. #4253

[UIKit] Update for Xcode10. #4253

Conversation

mandel-macaque commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 14, 2018

Build failure Build comment file:

Test results

Failed tests

mandel-macaque commented Jun 14, 2018

monojenkins commented Jun 14, 2018

Build failure Build comment file:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 14, 2018

Build failure Build comment file:

spouliot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalexsoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 14, 2018

Build failure Build comment file:

Test results

Failed tests

monojenkins commented Jun 15, 2018

Build failure Build comment file:

monojenkins commented Jun 15, 2018

Build failure Build comment file:

Test results

Failed tests

spouliot left a comment

Choose a reason for hiding this comment

VincentDondain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 18, 2018

Build success Build comment file:

monojenkins commented Jun 18, 2018

Build success Build comment file:

Build failure
Build comment file:

Build failure
Build comment file:

Build failure
Build comment file:

Build failure
Build comment file:

Build failure
Build comment file:

Build failure
Build comment file:

Build success
Build comment file:

Build success
Build comment file: